Show helpful hint when installing a stdlib module#13792
Show helpful hint when installing a stdlib module#13792
Conversation
Expose sys.stdlib_module_names (Python 3.10+) with a fallback to an empty frozenset for older Python versions. This will be used to provide helpful error messages when users try to install standard library modules.
When a user tries to install a standard library module and no matching distribution is found, show a helpful hint informing them that the module is built-in and can be imported directly. Only show the hint for top-level requirements, not for dependency resolution failures.
Apply the same stdlib module hint to the legacy resolver's find_requirement function. Only show for top-level requirements.
Show the same helpful hint when users query a stdlib module name using pip index versions.
- Unit tests for stdlib_module_names in compat module - Functional test for pip install with stdlib module - Functional test for pip index versions with stdlib module Tests are skipped on Python < 3.10 where sys.stdlib_module_names is not available.
|
Hi @notatallshaw this is my first contribution |
notatallshaw
left a comment
notatallshaw
left a comment
There was a problem hiding this comment.
Overall I'm +1 on this, I've seen this this mistake in real life, especially with new users of Python.
With regards to the PR I have a couple of issues:
- You've added the message in three places, but only two of them are tested, add a test for all three places it triggers, or remove the places you are not testing
- Commit da78132 includes Claude as a co-author, while using bots and LLM tools to write high quality PRs is fine, all commits are your own responsibility, and I don't want to see Claude automatically added to the
AUTHORS.txt(I understanddependabot[bot]andpre-commit-ci[bot]already are but they are more mechanical and less sensitive of a topic), please edit or rebase your commits so they are all your own
tests/unit/test_compat.py
Outdated
There was a problem hiding this comment.
This test isn't adding any value, please make sure tests are meaningful.
tests/unit/test_compat.py
Outdated
There was a problem hiding this comment.
Also this doesn't appear to add any value and can be removed.
src/pip/_internal/commands/index.py
Outdated
There was a problem hiding this comment.
I think if the exact same message is being used in three places we should put it in a common module somewhere, hopefully there is already an appropriate utility module for it to be placed.
There was a problem hiding this comment.
I don't love this message:
- The only place we use "Hint:" is in an error message and we don't use caps.
- I'm tempted to think we should make this a
warning, but I want to hear from other maintainers on that - The message is not always true, a user might install a local module (or remote from an alternative index) that has the same name as a standard library module, while this is bad practice it is technically valid (and may have a use case?).
My initial thought is something worded like:
logger.warning(
"%r is a Python standard library module name; it likely does not need to be "
"installed. Installing a package with the same name may override it and break imports.",
name,
)- Extract common warning to warn_stdlib_module() in compat.py (DRY) - Use logger.warning() instead of logger.info() with "HINT:" prefix - Adopt reviewer-suggested wording that's more nuanced about same-name packages potentially overriding stdlib - Remove low-value unit tests for stdlib_module_names in test_compat.py - Add missing unit test for legacy resolver path (package_finder.py) - Update functional tests to match new warning message
eureka928
force-pushed
the
stdlib-module-hint
branch
from
da78132 to
121634e
Compare
|
Thanks for the thorough review @notatallshaw! I've addressed all your feedback: Common module: Extracted the duplicated message into a warn_stdlib_module() helper in utils/compat.py (where stdlib_module_names already lives). All 3 call sites now use this single function. Message wording: Adopted your suggested approach — switched to logger.warning(), dropped the "HINT:" prefix, and used more nuanced wording that acknowledges same-name packages may legitimately exist: 'os' is a Python standard library module name; it likely does not need to be installed. Installing a package with the same name may override it and break imports. Tests: Removed the three low-value test_compat.py tests. Added a unit test in test_finder.py for the legacy resolver path (find_requirement), so all 3 locations are now covered. |
2 participants
When a user tries to
pip installa standard library module (e.g.,os,sys,json), pip now shows a helpful hint explaining that the module is built-in and can be imported directly.The hint is shown in the new resolver, legacy resolver, and
pip index versionscommand. It usessys.stdlib_module_names(Python 3.10+) with a graceful fallback to an empty frozenset on older versions.Closes #12731